-
Notifications
You must be signed in to change notification settings - Fork 6k
Update EIP-7723: Update 7723 to include EELS/EEST in CFI #9290
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
✅ All reviewers have approved. |
|
@marioevz I think we should make it a |
EIPS/eip-7723.md
Outdated
|
|
||
| An EIP **MAY** be moved from `Considered for Inclusion` to `Declined for Inclusion` if client teams are against including the EIP in the network upgrade. | ||
|
|
||
| An EIP **SHOULD** have a Python implementation in [execution-specs](https://github.com/ethereum/execution-specs) in an open PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing our team discussed was changing this to any client implementation, out of concern for overhead. A neutral (and arguably disposable) implementation of an EIP prior to it being considered is a high bar, and not strictly necessary to fill tests against. Our thoughts are nuanced on this matter though.
This doc could also provide some guidance on when it is ok for an exception to the SHOULD, and set the expectation that ACD rough consensus would be the arbiter of that exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appreciate the feedback, we could add a line regarding the CFI'd stage requirements on the basis of an ACD consensus.
Regarding the EELS overhead, the really nice thing about having everything in EELS from the start is that when the time comes to join all EIPs together to fill tests for the entire fork, everything is one implementation instead of scattered on different client implementations, because it's a big bottleneck to testing to fill one part of the tests with one t8n tool and then the rest with another.
|
The commit 4898448 (as a parent of 1866b43) contains errors. |
|
Pushed a change to include a paragraph about EELS and EEST changes required for EIPs that get updated and are already in CFI or SFI stage. |
eth-bot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All Reviewers Have Approved; Performing Automatic Merge...
This PR is currently in draft to start the discussion on how to incorporate EELS and EEST into EIP-7723.
Open questions:
There's still a discussion to be had in EELS and EEST teams to see if we are ready to start accepting contributions from EIP writers, and we can update this PR with a task list of the steps that need to happen prior to this PR being ready to merge.